[improvement](be) Implement native Parquet decoding for FileScannerV2#65674
Open
Gabriel39 wants to merge 16 commits into
Open
[improvement](be) Implement native Parquet decoding for FileScannerV2#65674Gabriel39 wants to merge 16 commits into
Gabriel39 wants to merge 16 commits into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Automated review completed. I found one issue in the FileScannerV2 Parquet reader changes: persistent scratch now extends Arrow binary chunk lifetimes after materialization/discard.\n\nValidation: static review only. git diff --check c8b81647776bcd482b99665530f9fd5ba41f0d03...0826614aff1d8bf5e21b78f8c8c5a0acfcb465de passed. I did not run build or unit/regression tests in this review-only checkout.
Contributor
TPC-H: Total hot run time: 29482 ms |
Contributor
TPC-DS: Total hot run time: 176989 ms |
Contributor
ClickBench: Total hot run time: 25.08 s |
Gabriel39
marked this pull request as ready for review
July 16, 2026 00:14
Gabriel39
added a commit
to Gabriel39/incubator-doris
that referenced
this pull request
Jul 16, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#65674 Problem Summary: FileScannerV2 retained Arrow binary and dictionary chunks in persistent Parquet reader scratch after synchronous Doris materialization. Large binary batches could therefore stay alive until a later read and overlap the next Arrow builder allocation. Release batch payload ownership on every success and error path while retaining vector capacity, clear borrowed StringRef scratch before its owners are released, and add unit coverage for ownership release and capacity reuse. Also remove the previous decoder changes under the legacy format directory so the v1 implementation remains unchanged; future native decoder work is constrained to format_v2. This lifecycle fix addresses the review finding but does not claim to eliminate the separate complex-column temporary materialization overhead. That v2-only decoder/materializer redesign remains distinct from COUNT(nullable_col) pushdown. ### Release note None ### Check List (For Author) - Test: Unit Test - Remote ASAN BE build - 16 focused Parquet v2 BE unit tests - Remote clang-format check - Remote clang-tidy attempted; blocked by existing diagnostics and the remote toolchain missing stddef.h during analysis - Behavior changed: No; internal buffer ownership is released earlier - Does this need documentation: Yes; updated the FileScannerV2 Parquet design document and format_v2 review guide
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review summary
Changes requested for one P1 memory-lifecycle regression. The allocation-reuse work is otherwise coherent, and the earlier Arrow-chunk ownership issue is fixed at the current head, but persistent nested scratch still lacks a bounded high-water release policy. See the inline comment.
Checkpoint conclusions
- Goal and proof: The PR reduces FileScannerV2/Parquet allocation and materialization overhead and hardens several reader boundaries. The changed tests cover logical reset, object/capacity reuse, Arrow-owner release, dictionary validation, column mapping, and split Row Group accounting, but they do not prove that oversized nested scratch is released; that missing property is the finding.
- Scope: The scanner, mapper, reader, statistics, Iceberg, tests, and design/guidance changes are focused on the stated optimization/hardening work.
- Concurrency: Row Group reader trees are uniquely owned and invoked serially by the scanner; no new shared globals, locks, atomics, lock ordering, or reentrant access was found.
- Lifecycle: Arrow chunks and borrowed
StringRefs now unwind safely on success/error paths, resolving the existing thread. However, Doris-owned nested value/null/conversion/level/index backing allocations remain until Row Group teardown; the inline P1 covers this distinct defect. - Configuration: No configuration items or dynamic-reload behavior are added.
- Compatibility: No storage-format, wire-protocol, symbol, or rolling-upgrade contract changes were found. Required-slot mapping is populated from the scan tuple for finalized and late-filter expressions, so removing the invalid ordinal fallback is safe.
- Parallel paths: Parquet, ORC, text, JSON/native layout consumers continue to use stable
local_positions; Scanner retains residual evaluation after lazy materialization. Dictionary, selection/page-skip, nested values/levels-only, List/Map/Struct, and Iceberg delete paths were also traced. - Conditions and errors: New slot/dictionary/selection validation returns checked errors; malformed partial reader state is terminal rather than retried. No speculative defensive continuation was introduced.
- Test coverage: Unit coverage is broad for the changed local behaviors, including negative dictionary/selection cases, but needs the oversized repeated-value retained-allocation regression test requested inline.
- Test results: No generated expected-result file changed. This review was static-only because the review contract prohibits builds in this runner.
- Observability: Existing Status context and profile counters are sufficient for these local changes; Row Group candidate totals are corrected per Split and no new critical metric/log path is required.
- Transactions and persistence: Not applicable; no transaction, EditLog, replay, or persistent metadata behavior changes.
- Data writes: Not applicable; scan-side decoding and equality-delete filtering do not alter write atomicity or crash recovery.
- FE/BE variables: No new transmitted variable or protocol field is introduced.
- Performance: Normal-size scratch reuse removes hot-path allocations, but child-count-unbounded nested buffers can accumulate outlier high-water capacity and cause a new memory-limit failure. Other reviewed CPU/I/O/allocation paths showed no substantiated regression.
- Other issues / user focus: No additional review focus was supplied. After three normal plus risk-focused convergence rounds, no second valuable finding or unresolved suspicious point remains.
Validation state
The live head is 913123e6cfeaa5712fd9571006a92e87f177f5a7. The author reports a remote ASAN build and focused unit tests, but I did not independently run them. Current formatter, license, FE UT, and Cloud UT checks pass. COMPILE, BE UT, and performance fail during CI setup because this head conflicts with current master in be/test/format_v2/parquet/parquet_reader_control_test.cpp; those jobs did not reach compilation/tests, and the PR is currently marked conflicting.
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: FileScannerV2 currently materializes Parquet values through Arrow and repeatedly allocates conversion and nested-column scratch. Introduce an Arrow-independent physical column schema and selection-aware flat decode contract in the existing Doris Parquet kernel, batch string materialization, retain scalar and complex-reader scratch across batches, and document the target shared-level-plan interface for native complex decoding. The legacy v1 production call path remains unchanged.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- Remote ASAN BE build
- 120 targeted Parquet BE unit tests
- Behavior changed: No; this adds migration interfaces and internal allocation optimizations
- Does this need documentation: Yes; updated the FileScannerV2 Parquet design document and format_v2 review guide
### What problem does this PR solve? Issue Number: None Related PR: apache#65674 Problem Summary: FileScannerV2 retained Arrow binary and dictionary chunks in persistent Parquet reader scratch after synchronous Doris materialization. Large binary batches could therefore stay alive until a later read and overlap the next Arrow builder allocation. Release batch payload ownership on every success and error path while retaining vector capacity, clear borrowed StringRef scratch before its owners are released, and add unit coverage for ownership release and capacity reuse. Also remove the previous decoder changes under the legacy format directory so the v1 implementation remains unchanged; future native decoder work is constrained to format_v2. This lifecycle fix addresses the review finding but does not claim to eliminate the separate complex-column temporary materialization overhead. That v2-only decoder/materializer redesign remains distinct from COUNT(nullable_col) pushdown. ### Release note None ### Check List (For Author) - Test: Unit Test - Remote ASAN BE build - 16 focused Parquet v2 BE unit tests - Remote clang-format check - Remote clang-tidy attempted; blocked by existing diagnostics and the remote toolchain missing stddef.h during analysis - Behavior changed: No; internal buffer ownership is released earlier - Does this need documentation: Yes; updated the FileScannerV2 Parquet design document and format_v2 review guide
### What problem does this PR solve? Issue Number: None Related PR: apache#65674 Problem Summary: FileScannerV2 materialized ordinary Parquet values through Arrow arrays and temporary nested batches, which added allocation, conversion, and peak-memory overhead compared with the v1 reader. Route ordinary predicate and output columns through a persistent native reader that decodes directly into Doris columns, preserves selection and page-index coordinates, shares v1-compatible footer/page caches and MergeRange I/O, and keeps Arrow only for metadata planning, dictionary probing, and the existing COUNT complex levels-only path. Add detailed native decode profiles, adaptive-batch fragmentation profiles, complex/string/decimal/fixed-binary coverage, and update the Parquet design and review guidance. No FE or COUNT pushdown behavior is changed, and no be/src/format decoder code is modified. ### Release note Improve FileScannerV2 Parquet scan memory use and native decode performance for scalar and complex columns. ### Check List (For Author) - Test: Unit Test - 192 format v2 Parquet BE unit tests on the designated remote host - Behavior changed: Yes (ordinary FileScannerV2 Parquet values use native direct materialization; query results and pushdown semantics are unchanged) - Does this need documentation: Yes (design and review guide updated in this change)
### What problem does this PR solve? Issue Number: close #xxx Related PR: apache#65674 Problem Summary: The Parquet v2 native scan path no longer uses the prototype Arrow value-reader hierarchy, but its decoded leaf batch, nested load/build/consume protocol, factories, and tests remained reachable from a shape-only aggregate path. Remove that intermediate layer and isolate the unchanged COUNT(nullable_col) level-reading compatibility path behind a narrow API. Ordinary scans now expose only the persistent native read/skip/select contract. ### Release note None ### Check List (For Author) - Test: Unit Test (remote validation pending) - Behavior changed: No - Does this need documentation: Yes (updated in this commit)
### What problem does this PR solve? Issue Number: None Related PR: apache#65674 Problem Summary: FileScannerV2 still paid for an Arrow-decoded value layer or decoder-owned logical conversion before constructing Doris columns. This duplicated buffers for strings and nested values, amplified adaptive-batch memory peaks, and made dictionary, page-cache, and decode profiles difficult to compare with the v1 reader. Introduce an independent Parquet page/encoding reader under format_v2 whose decoders only parse encoded streams and own cursors. DataTypeSerDe now interprets Parquet physical and logical annotations and materializes selected values directly into persistent Doris columns. The native reader supports the existing scalar, decimal, date/time, timestamp, UUID, dictionary, delta, byte-stream-split, Page V1/V2, nested level, selection, page-index, footer/page-cache, and MergeRange paths. COUNT complex columns use a levels-only reader with persistent scratch, and page/decode/cache profiles expose cumulative deltas without duplicate counting. Arrow remains only in metadata/index planning and test fixture generation. No FE code or v1 format decoder is modified. ### Release note Improve FileScannerV2 Parquet scan memory use and native decode performance, especially for strings, decimals, nested columns, dictionary selection, and adaptive batch sizes. ### Check List (For Author) - Test: Unit Test - 14 Parquet decoder and DataTypeSerDe tests on the designated remote host - 76 Parquet v2 reader, selection, page-index, complex-column, and scan tests on the designated remote host - 6 focused COUNT/profile tests on the designated remote host - Behavior changed: Yes (FileScannerV2 Parquet data pages materialize through the native v2 decoder and DataTypeSerDe; query semantics are unchanged) - Does this need documentation: Yes (updated the FileScannerV2 Parquet design and review guide)
Gabriel39
force-pushed
the
codex/fix-file-scanner-v2-native-decode
branch
from
July 16, 2026 11:36
913123e to
78678d9
Compare
### What problem does this PR solve? Issue Number: None Related PR: apache#65674 Problem Summary: Split the Parquet v2 decoder factory by encoding family and replace raw allocations with std::make_unique. This keeps decoder construction behavior unchanged while satisfying function-size and ownership static-analysis rules. ### Release note None ### Check List (For Author) - Test: Unit Test - Remote ASAN BE build and 90 Parquet v2/SerDe unit tests - Remote clang-tidy on the changed decoder factory - Behavior changed: No - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#65674 Problem Summary: Persistent Parquet v2 leaf readers reused level, null, selection, conversion, and dictionary-index scratch across batches. A legal repeated column can expand one logical batch into an exceptional number of leaf values, so staggered outliers could retain large allocations in multiple leaves until Row Group teardown. Keep normal-size capacity reusable, recursively release individual scratch buffers above a 4 MiB high-water mark after the top-level complex reader consumes the level plan, and cover the retained-capacity drop with a unit test. ### Release note None ### Check List (For Author) - Test: Unit Test - Remote ASAN ./build.sh --be - Remote Parquet v2 and SerDe unit-test suite - Behavior changed: No - Does this need documentation: Yes. Updated the FileScannerV2 Parquet design and review guide.
Gabriel39
changed the title
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review summary
Changes requested for 15 new, non-duplicate issues in the native Parquet reader. They cluster in three boundaries: shared footer/page-cache identity and immutability; exact decoder/page/level validation; and ARRAY/MAP/STRUCT shape preservation. The earlier Arrow-chunk ownership issue is fixed, and the existing nested-scratch high-water P1 remains active without being duplicated here.
Checkpoint conclusions
- Goal and proof: The PR substantially implements native decoding and removes the old intermediate value path, but the current tests do not prove safe behavior for valid Page V1 BIT_PACKED levels, cache-order/version changes, decoder reuse, or several malformed-page/nested-shape cases covered by the inline findings.
- Scope: The production, test, and design changes are focused on the stated FileScannerV2 Parquet migration, though the migration is broad enough that cache, decoder, selection, COUNT, and complex-column boundaries all needed end-to-end tracing.
- Concurrency: Concurrent scans can share a cached
FileMetaData; post-publicationassign_ids()mutates it without synchronization. No other new lock-order or thread-entry issue was substantiated. - Lifecycle: Footer metadata is both option-dependent and mutated after publication; native S3 page keys lose the split's stable version; the reused delta-length decoder keeps stale/uninitialized state on empty pages. The existing nested-scratch retention thread remains separate.
- Configuration: No new configuration item is added, but existing VARBINARY/TIMESTAMPTZ mapping options affect parsed schema and are missing from the shared footer-cache identity.
- Compatibility: No storage or wire format is changed, but valid Page V1 BIT_PACKED definition/repetition levels are not supported by the non-bulk cursor paths, so existing files can fail or execute undefined behavior.
- Parallel paths: Page V1/V2, compressed/uncompressed and cache-hit reads, selected/filtered/skipped values, normal nested scans and COUNT, v1/v2 footer-cache producers, and MAP/STRUCT sibling paths were checked. The findings identify the parallel instances that need the same exact validation.
- Conditions and errors: Several decoder return values/counts are discarded, signed external sizes reach unsafe arithmetic/buffers, a malformed BYTE_STREAM_SPLIT payload reaches
DORIS_CHECK, and release-onlyDCHECKs stand in for required corruption statuses. - Test coverage: Happy-path coverage is broad, but negative tests are missing for truncated BOOLEAN/delta/level streams, page-size and V2-level layout corruption, empty reused delta-length pages, null/misaligned MAPs, short siblings, leading continuations, and cache-order/version behavior. A concurrent same-footer cache test and valid BIT_PACKED Page V1 tests are also needed.
- Test results: No generated regression
.outfile changed. The author reports remote ASAN build/focused-unit/format validation in the PR history; I did not independently run builds or tests under this review contract. - Observability: No new metric is required for these local failures; checked corruption statuses with file/column/page context are preferable to silent success, undefined behavior, or process termination.
- Transactions and persistence: Not applicable; this is scan-side parsing/decoding and cache state, with no EditLog, replay, or transaction change.
- Data writes: Not applicable; no table-data write, publish, or crash-recovery path is modified.
- FE/BE variables: No new FE-to-BE transmitted variable or protocol field is introduced.
- Performance: The native/materialization design removes intended allocations and copies, but correctness and bounded lifecycle must hold first. The existing P1 already covers retained outlier scratch; no additional non-duplicate performance regression was found.
- Other issues / user focus: No extra user focus was supplied. After three normal/risk convergence rounds, the 15 inline findings below are the complete current non-duplicate set.
Validation state
The reviewed live head is d257ca06abff17c3a43f6f287594dc8dcf52ef80. This was static review only: the supplied contract prohibits builds, and this checkout has no .worktree_initialized, thirdparty/installed, or thirdparty/installed/bin/protoc.
Contributor
TPC-H: Total hot run time: 29697 ms |
### What problem does this PR solve? Issue Number: None Related PR: apache#65674 Problem Summary: Highly selective Parquet v2 scans entered DataTypeSerDe and the encoding decoder once per selection run. Fragmented selections therefore amplified virtual dispatch, consumer construction, and cursor-management overhead. Add a page-fragment hybrid selection plan that enters SerDe once and lets each native encoding batch gather or compact selected values while preserving and validating the full physical cursor. Add profile counters, decoder coverage, scratch retention bounds, and design/review documentation. ### Release note None ### Check List (For Author) - Test: No test run (per user request) - Behavior changed: Yes (sparse Parquet v2 materialization is batched per page fragment) - Does this need documentation: Yes (included in this PR)
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 29840 ms |
Contributor
TPC-DS: Total hot run time: 178306 ms |
Contributor
ClickBench: Total hot run time: 25.17 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
TPC-H: Total hot run time: 29480 ms |
Contributor
TPC-DS: Total hot run time: 177428 ms |
Contributor
ClickBench: Total hot run time: 25.12 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Request changes. I found three new issues in the native Parquet path: two P1 optional-index boundary failures (an unbounded serialized-index allocation and false-negative IS NULL pruning from contradictory null metadata) plus one P2 cache-disabled V2 level-copy regression.
Checkpoint summary:
- Goal, scope, and tests: the Arrow-to-native materialization goal is coherent and the PR adds broad SerDe, decoder, nested-reader, scheduling, cache, compatibility, and profile coverage. The three inline cases still need focused fallback/correctness/performance tests.
- Correctness and compatibility: I checked flat/nested, plain/dictionary, indexed/sequential, selected/skipped, page-crossing, strict/non-strict, old-writer, and compressed/uncompressed V2 paths. Existing cardinality, decoder, decimal, timestamp, and projection findings were deduplicated rather than repeated.
- Concurrency, lifecycle, and observability: shared metadata/cache publication and cleanup/profile paths were reviewed. No additional distinct concurrency issue survived; the current lifecycle/profile threads remain applicable.
- Configuration and performance: cache enable/disable and mapping/compatibility options were traced. The cache-disabled V2 copy and optional-index memory exposure are the new surviving performance/resource issues.
- Transactions, writes, and FE/BE variable propagation: not applicable to this read-path change.
- User focus: no additional focus was provided.
This was a static review as required by the runner contract; I did not build or run tests. All 134 authoritative changed paths were swept, and the independent full/risk reviews converged on this exact comment set with no further valuable findings.
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
The native Parquet migration still has ten blocking correctness and robustness issues: malformed optional Bloom metadata can cause an out-of-bounds probe or multi-gigabyte allocation; schema child counts, unbounded schema depth, and row-group chunk cardinality can abort or exhaust the BE; split assignment can silently omit rows and prefetch planning can abort because both bypass the checked chunk-range contract; invalid logical/physical annotations reach fatal direct-consumer assertions; inconsistent footer null counts can falsely prune real rows; the first V1 nested page can silently discard orphan continuation values; and an unrelated Nereids golden-file edit has no changed producer.
Critical checkpoints: correctness/data integrity is blocked by the footer-statistics and logical-type cases; process safety/resource bounds is blocked by the Bloom/schema/prefetch cases; scanner lifecycle, lazy materialization, profile propagation, cache/IO integration, compatibility paths, and concurrency were reviewed with no additional non-duplicate findings beyond existing threads and this inline set. The new tests are broad but omit the malformed-metadata cases requested inline. Builds/tests were not run because this automated review environment explicitly prohibits them. The user supplied no additional focus beyond the complete review.
Existing review threads and replies were deduplicated before selecting these comments.
Convergence note: the tenth candidate was found and independently verified in the third and final configured review round. It is included below, but the three-round cap means this review cannot claim a subsequent same-set convergence pass.
Contributor
TPC-H: Total hot run time: 29774 ms |
Contributor
TPC-DS: Total hot run time: 177413 ms |
Contributor
ClickBench: Total hot run time: 24.98 s |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
TPC-H: Total hot run time: 29294 ms |
Contributor
TPC-DS: Total hot run time: 177253 ms |
Contributor
ClickBench: Total hot run time: 25.13 s |
Contributor
There was a problem hiding this comment.
Request changes. After deduplicating all 80 existing inline comments, this review found six new P1 correctness/compatibility failures and one P2 profiling defect in the native FileScannerV2 Parquet path.
Critical checkpoint conclusions:
- Goal and scope: the Arrow-to-native decode/materialization direction is coherent, but the full 145-file change is not yet safe to merge because valid or malformed Parquet metadata can still abort the BE, select the wrong schema/page layout, or return wrong values.
- Correctness and error handling: the blocking cases are the obsolete raw page-cache range assertion, missing PageHeader and SchemaElement discriminant validation, and a 32-bit DELTA padding-cursor wrap. External file metadata must return a checked error rather than assert or silently shift the decoder cursor.
- Compatibility and schema inference: valid ENUM/BSON leaves regress from STRING to VARBINARY, and valid logical TIME(MILLIS/MICROS) is inferred as TIMEV2(0), hiding fractional precision.
- Architecture and maintainability: the native trust-boundary validators need to own the file-format invariants before downstream decoders consume generated Thrift defaults; duplicated schema mapping should remain aligned with the established resolver.
- Concurrency and lifecycle: cache publication, row-group reader ownership, periodic profile flushing, close/reset, and merge-reader lifetime were rechecked; no additional non-duplicate race, deadlock, use-after-free, or lost additive profile delta survived review.
- Performance and observability: the native/direct-materialization design targets the intended CPU and memory reductions, but PageCrossingBatches is currently reconstructed per 16-batch profile flush and per column, so it cannot be compared with per-batch TotalBatches as documented.
- Tests and validation: changed tests are broad but omit the seven inline boundary cases. This was a static review as required by the runner contract; no local build or tests were run. Current CI has compile/style/license/performance and vault checks passing, Linux BE UT/regressions still pending, and the macOS BE UT job failed at setup because the runner supplied Java 25 while the workflow requires JDK 17.
- User focus: no additional focus was supplied, so the complete authoritative patch, mandatory design docs, existing threads, parallel paths, and error/resource boundaries were reviewed.
All three final-round agents independently returned NO_NEW_VALUABLE_FINDINGS for this exact seven-comment set at head f1764a7.
| DORIS_CHECK(leaf_column_id >= 0 && | ||
| leaf_column_id < static_cast<int>(row_group_metadata.columns.size())); | ||
| const auto& column_metadata = row_group_metadata.columns[leaf_column_id].meta_data; | ||
| const int64_t offset = column_metadata.__isset.dictionary_page_offset |
Contributor
There was a problem hiding this comment.
[P1] Remove or validate this obsolete page-cache range traversal The new metadata-safety test deliberately permits an optional dictionary_page_offset = -1 by falling back to the valid data-page offset in build_native_prefetch_ranges(). This separate open-time helper instead selects the set dictionary offset unconditionally and DORIS_CHECKs it, so enabling the Parquet page cache turns that same file into a BE abort. register_page_cache_ranges() now discards the result because native readers cache exact pages, so remove this traversal or reuse the checked chunk-range helper and propagate a Status rather than asserting on file metadata.
| } | ||
| } else if (logicalType.__isset.FLOAT16) { | ||
| ans.first = DataTypeFactory::instance().create_data_type(TYPE_FLOAT, nullable); | ||
| } else { |
Contributor
There was a problem hiding this comment.
[P1] Preserve STRING inference for ENUM and BSON leaves This mapper omits logical ENUM/BSON, and the converted-type overload omits their legacy annotations too. Both cases therefore throw into get_doris_type()'s BYTE_ARRAY fallback; V2 enables varbinary mapping, so valid ENUM/BSON columns are now exposed as VARBINARY. The established parquet_type.cpp resolver and this native decoder both classify these annotations as STRING. Handle ENUM/BSON alongside STRING/JSON in both overloads and cover valid annotated leaves, not only the invalid ENUM-group case.
| } | ||
| } | ||
| } else if (logicalType.__isset.TIME) { | ||
| ans.first = DataTypeFactory::instance().create_data_type(TYPE_TIMEV2, nullable); |
Contributor
There was a problem hiding this comment.
[P1] Derive TIMEV2 scale from the Parquet time unit The factory default here is scale 0, while valid logical TIME(MILLIS/MICROS) carries fractional precision and the native decoder preserves that unit and materializes microseconds. The established parquet_type.cpp resolver reports TIMEV2(3) and TIMEV2(6); returning TIMEV2(0) from the new inferred schema suppresses those fractional digits at serialization. Map the unit to the same scale and add inferred-schema/value coverage for both units.
| COUNTER_UPDATE(_profile.native_page_fragments, page_fragments); | ||
| } | ||
| // Summed fragments from MAP/STRUCT siblings do not mean any individual leaf crossed a page. | ||
| if (max_leaf_page_reads > 1 && _profile.page_crossing_batches != nullptr) { |
Contributor
There was a problem hiding this comment.
[P2] Count page crossings at the scheduler batch boundary flush_profile() runs only every 16 scheduler batches (or at the row-group tail), so max_leaf_page_reads > 1 describes an amortized interval, not one batch. Sixteen batches that each open one page become one false crossing, sixteen real crossings become one, and each projected top-level column can increment the same shared counter. This makes PageCrossingBatches incomparable with per-batch TotalBatches as documented. Record/OR a crossing flag for each scheduler batch and keep the periodic flush only for additive cumulative statistics.
| // Sizes are untrusted signed Thrift fields and must fit the column chunk before arithmetic. | ||
| return Status::Corruption("Parquet page payload exceeds its column chunk"); | ||
| } | ||
| if (_cur_page_header.__isset.data_page_header_v2) { |
Contributor
There was a problem hiding this comment.
[P1] Validate the page-type discriminant before choosing a layout PageHeader.type is the contract that identifies which single page-specific header is set, but this validation and is_header_v2() choose V2 solely from __isset.data_page_header_v2. A DATA_PAGE_V2 carrying only a V1 member is decoded as V1, a DATA_PAGE carrying a V2 member is decoded as V2, and absent/dual members are not rejected before level, compression, and payload cursors are selected. Require the matching member (and reject competing members) for each page type, with malformed swapped/absent/dual-header tests.
|
|
||
| const size_t depth = pending.back().depth + 1; | ||
| --pending.back().remaining_children; | ||
| const int children = num_children_node(schemas[pos]); |
Contributor
There was a problem hiding this comment.
[P1] Validate SchemaElement kind and repetition before parsing This pass bounds num_children, but it never enforces the Parquet contract that groups set num_children and no type, primitives set type and no num_children, and every non-root node sets repetition_type. With neither kind field, the node is treated as a leaf and the generated default type (BOOLEAN) is consumed; with no repetition bit it is treated as REQUIRED. For an optional Page V1 leaf that can leave definition-level bytes at the value cursor instead of returning corruption. Validate these isset invariants here before building the tree and add missing/dual-kind plus missing-repetition footer tests.
| _total_values_remaining -= num_values; | ||
|
|
||
| if (_total_values_remaining == 0) [[unlikely]] { | ||
| if (!_bit_reader->Advance(_delta_bit_width * _values_remaining_current_mini_block)) { |
Contributor
There was a problem hiding this comment.
[P1] Widen the final miniblock padding calculation The multiplication occurs in uint32_t before the result reaches Advance(int64_t). For example, block size 134217856 with one miniblock, two values, and width 32 requires skipping 4,294,971,360 bits after the second value, but this expression wraps to 4,064 bits. That block still fits the signed page-size domain; in DELTA_LENGTH_BYTE_ARRAY the shared locator is then 512 MiB early and returns padding as string payload. Use checked uint64/int64 multiplication, verify it fits the remaining stream, and cover the overflow boundary plus the byte-array cursor handoff.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary:
FileScannerV2 previously used Arrow RecordReader/arrays/builders for Parquet value materialization and reconstructed complex columns through intermediate decoded objects. That path duplicated decode/materialization work, retained fragmented per-batch scratch, amplified string and nested-column memory usage, and made adaptive batches and cache/profile behavior diverge from the legacy v1 reader.
This PR implements an independent native Parquet read path under
format_v2:HybridSelectionBatches,HybridSelectionRanges, andHybridSelectionNullFallbackBatchesfor diagnosis.be/src/formatv1 implementation unchanged.Arrow is now limited to the existing metadata/index/probe adapter boundary; it is not used to materialize scan values.
Release note
FileScannerV2 Parquet scans now use native Doris page/encoding decoding and direct DataTypeSerDe materialization instead of Arrow value readers.
Check List (For Author)
d257ca06abf: remote ASAN BE build passed; targeted BE UT passed 91/91; LLVM 16 format check passed.099c111c12fadds hybrid sparse decoding and decoder UT coverage. Build/UT validation was not run at the user's request; CI is requested below.docs/file-scanner-v2-parquet-scan-design.mdanddocs/file-scanner-v2-code-review-guide.md.